Conversation
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
There was a problem hiding this comment.
Pull request overview
Replaces several TODOs with clearer inline documentation/Javadoc and adds a small set of unit tests to cover Histogram.Builder#nativeResetDuration(...) validation/configuration.
Changes:
- Expanded TODO comments into explanatory notes (OpenTelemetry exporter filtering, Summary exemplars, SlidingWindow thread-safety/perf).
- Added tests for
nativeResetDuration(...)positive/zero/negative inputs. - Added class and builder-method Javadocs for
ExporterOpenTelemetryProperties.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| prometheus-metrics-exporter-opentelemetry/src/main/java/io/prometheus/metrics/exporter/opentelemetry/PrometheusMetricProducer.java | Replaces TODO with guidance on how filtering could be added in the future. |
| prometheus-metrics-core/src/test/java/io/prometheus/metrics/core/metrics/HistogramTest.java | Adds tests around nativeResetDuration(...). |
| prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Summary.java | Documents current behavior/limitations for Summary exemplars. |
| prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/SlidingWindow.java | Improves class-level documentation about thread safety and performance. |
| prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Histogram.java | Removes a TODO related to missing reset-duration tests. |
| prometheus-metrics-config/src/main/java/io/prometheus/metrics/config/ExporterOpenTelemetryProperties.java | Adds comprehensive Javadoc for properties and builder methods. |
Comments suppressed due to low confidence (1)
prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Histogram.java:969
- nativeResetDurationSeconds is derived via unit.toSeconds(duration), which truncates sub-second values to 0. That means calls like nativeResetDuration(500, MILLISECONDS) silently disable resets despite passing the duration > 0 validation. Consider either rejecting durations that round to 0 seconds, documenting second-level granularity, or rounding up to at least 1 second.
public Builder nativeResetDuration(long duration, TimeUnit unit) {
if (duration <= 0) {
throw new IllegalArgumentException(duration + ": value > 0 expected");
}
nativeResetDurationSeconds = unit.toSeconds(duration);
return this;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // While exemplars are sampled during observe() calls (see lines 175, 188), | ||
| // they are not included in the snapshot to maintain consistency with the buffering | ||
| // mechanism. The buffer.run() ensures atomic collection of count, sum, and quantiles. | ||
| // Adding exemplars would require coordination between the buffer and exemplarSampler, | ||
| // which could impact performance. Consider using Histogram instead if exemplars are | ||
| // needed. |
There was a problem hiding this comment.
The comment references specific source line numbers ("see lines 175, 188"), which will become stale as the file changes. Prefer referencing the relevant methods/fields (e.g., observe()/observeWithExemplar() and exemplarSampler) instead of absolute line numbers.
| // While exemplars are sampled during observe() calls (see lines 175, 188), | |
| // they are not included in the snapshot to maintain consistency with the buffering | |
| // mechanism. The buffer.run() ensures atomic collection of count, sum, and quantiles. | |
| // Adding exemplars would require coordination between the buffer and exemplarSampler, | |
| // which could impact performance. Consider using Histogram instead if exemplars are | |
| // needed. | |
| // While exemplars are sampled during observe() and observeWithExemplar() via | |
| // exemplarSampler, they are not included in the snapshot to maintain consistency | |
| // with the buffering mechanism. The buffer.run() ensures atomic collection of | |
| // count, sum, and quantiles. Adding exemplars would require coordination between | |
| // the buffer and exemplarSampler, which could impact performance. Consider using | |
| // Histogram instead if exemplars are needed. |
| /** | ||
| * The interval between the start of two export attempts. Default is 60000. | ||
| * | ||
| * <p>Like OpenTelemetry's <a | ||
| * href="https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#periodic-metric-reader">OTEL_METRIC_EXPORT_INTERVAL</a>, | ||
| * but in seconds rather than milliseconds. |
There was a problem hiding this comment.
The Javadoc says "Default is 60000" for an API that accepts seconds; that reads as 60000 seconds (~16.7h). If the intended default is OpenTelemetry's 60000ms (i.e., 60 seconds), update the wording/units to avoid misleading users.
| HistogramSnapshot snapshot = histogram.collect(); | ||
| assertThat(snapshot.getDataPoints()).hasSize(1); | ||
| assertThat(snapshot.getDataPoints().get(0).hasNativeHistogramData()).isTrue(); | ||
| } |
There was a problem hiding this comment.
This test doesn't assert anything specific about the configured reset duration; it would still pass if nativeResetDuration(...) were a no-op (nativeOnly() already yields native histogram data). Consider asserting the configured duration is propagated (e.g., via reflection on the built Histogram / MetricsProperties) or exercising the reset scheduling/behavior in a deterministic way.
| * observations. Potential optimizations include: | ||
| * | ||
| * <ul> | ||
| * <li>Using {@link java.util.concurrent.locks.ReadWriteLock} to allow concurrent observations | ||
| * <li>Using lock-free data structures with {@link java.util.concurrent.atomic atomic} operations | ||
| * <li>Implementing a lock-free ring buffer with striped buckets | ||
| * </ul> |
There was a problem hiding this comment.
maybe this one could be converted into an issue if we want to improve it, or we just remove the TODO comment entirely? I'm not sure the addition of this new comment is all that useful
2 participants
No description provided.